feat: preliminary support for //extensions #17440

samuelmaddock · 2019-03-18T16:55:41Z

Description of Change

Overview

These are the initial, exploratory changes for adding support for Chrome extensions using the //extensions dependency from Chromium's source code. This dependency is meant to allow embedders of Chromium the ability to add extension support for their project.

Within the Electron community, there are several projects with their own implementation of Chrome extensions. No such implementation is complete and ends up duplicating the work of other implementations.

The goal of these changes are to introduce the first steps towards a more complete, official implementation of Chrome extensions for Electron-based projects.

Summary of changes

What's included

Loading of unpackaged Chrome extension
Content script injection
World isolation (aka context isolation) of content scripts
Content script list in DevTools
Extension manifest parsing and validation

What's not included (yet)

Background scripts
DevTools extensions
Extension badges
Popup windows for extension badges

What won't be included

Chrome platform apps
.CRX archive loader

Usage

Currently an extension can be loaded by providing a --load-extension=/absolute/path/to/extension switch. This is planned to be superseded by JavaScript bindings prior to merging.

Architecture

The file and code additions to this PR are mostly sourced from the //src/extensions/shell project.

app_shell is an experimental project to build a minimal environment like content_shell.
The goal is to be able to run a v2 app and supply most of the chrome.* extension APIs without running the rest of Chrome.

In other words, the changes in this PR are mostly necessary boilerplate copied from app_shell to embed extensions.

app_shell implements support for Chrome Apps while we want support Chrome Extensions (differences). Luckily they both share a majority of the same core APIs. The main addition we required was to create an instance of SharedUserScriptMaster. The history of commits provide a good timeline of APIs added.

Below are some of the notable classes added or changed as part of embedding extensions.

Browser

The entry point of changes for the browser process can be found in atom_browser_main_parts.cc.

Class	Description
`SharedUserScriptMaster`	Manages shared memory for communicating content scripts changes between the browser and renderer processes.
`AtomExtensionSystem`	Initializes core extension APIs for the browser process. Responsible for creating an instance of `SharedUserScriptMaster`. Globally accessible using `extensions::ExtensionSystem::Get(browser_context)`. Will likely be used by Electron JavaScript bindings for managing extensions.
`AtomExtensionLoader`	Provides functionality for loading and reloading extensions.
`AtomExtensionsBrowserClient`	Interface to allow the extensions module to make browser-process-specific queries of Electron. This is where we can ignore specific features of the extensions module such as platform apps, profiles, and background updates.
`AtomBrowserContext`	Now responsible for managing extension preferences. This required switching from using `PrefRegistrySimple` and `PrefRegistrySyncable` (which inherits `PrefRegistrySimple`).

Common

Class	Description
`AtomExtensionsClient`	Electron-variant of `ExtensionsClient` singleton for providing common global APIs between the browser and renderer process. This is where we can add what `chrome.*` API features we choose to support for Electron.

There's a bit of code generation that happens for implementing common extension features. This is done by atom/common/extensions/api/BUILD.gn which sources _manifest_features.json. For now, this only generates a manifest feature for content scripts. The config came from Chrome's _manifest_features.json.

Renderer

The entry point of changes for the renderer process can be found in renderer_client_base.cc.

Class	Description
`AtomExtensionsRendererClient`	Provides Electron-specific hooks into the extensions system for the renderer. Communicating to its `extensions::Dispatcher` reference sends events to the rest of the extensions system.

Bindings API design (WIP)

As extensions seem to be tied closely to a BrowserContext, it might be a good idea to provide methods on the Session API. This is the approach that the Muon fork took.

interface Extension {
  id: string,
  path: string,
  manifest: chrome.Manifest
}

class Session {
  /** Emitted when extension is installed and ready. */
  on(event: 'extension-ready', listener: (event: Event, extension: Extension) => void): this;

  loadExtension(path: string): Extension;
  removeExtension(extensionId: string): void;
  enableExtension(extensionId: string): void;
  disableExtension(extensionId: string): void;
}

Roadmap to merging

Get initial feedback from maintainers
Add #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS) guards
Fix tests
Rename Shell* classes to Atom*

Followup roadmap

Add support for extensions in multiple BrowserContexts
Electron bindings for extension management (see Bindings API design)
Feature parity with existing JS extension implementation
- Background scripts
- DevTool extensions
Deprecate JS extensions implementation
Remove JS extensions implementation
Documentation for supported chrome.* APIs
Allow runtime toggling of extensions support?

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR title follows semantic commit guidelines
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: no-notes

Initial merge won't allow developers to use these features just yet.

ckerr · 2019-03-27T19:09:54Z

I know this is still a WIP but @'ing wg-upgrades anyway to get feedback from that team anyway for feedback on the idea of this feature in electron/electron

ckerr

This review was done as a group at the hackweek.

atom/browser/atom_browser_context.cc

ckerr · 2019-04-24T17:49:34Z

atom/browser/atom_browser_context.cc

+    base::OnceClosure closure) {
+  // This method is called for Extension supports, but tests do not need to
+  // support exceptional CORS handling.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, std::move(closure));


Could a // TODO be added here -- we probably do need to support these allow/block patterns

atom/browser/atom_browser_main_parts.cc

atom/common/extensions/api/_manifest_features.json

ckerr · 2019-04-24T18:31:24Z

atom/common/extensions/atom_extensions_api_provider.cc

+AtomExtensionsAPIProvider::AtomExtensionsAPIProvider() = default;
+AtomExtensionsAPIProvider::~AtomExtensionsAPIProvider() = default;
+
+// TODO(samuelmaddock): generate API features?


We don't understand this comment. What needs to be done here?

AddShellAPIFeatures is a code generated function sourced from _api_features.json. The extensions subsystem and Chrome have their own features. We might need to do the same here.

AddAtomManifestFeatures, on line 32, is generated by the BUILD.gn file in this directory.

ckerr · 2019-04-24T18:35:11Z

atom/common/extensions/atom_extensions_client.cc

+
+const std::string AtomExtensionsClient::GetProductName() {
+  // TODO(samuelmaddock):
+  return "app_shell";


this should use atom again

The issue here is that this in in common and therefore can run in either process. We can't guarantee to have access to atom::Browser::Get()->GetName()

cc @deepak1556 @nornagon suggestions?

where does this string end up? maybe just Electron is sufficient?

ckerr · 2019-04-24T18:38:06Z

atom/renderer/extensions/atom_extensions_renderer_client.cc

+  // extensions, so we always return 1. Note that 0 is reserved for the global
+  // world.
+  // TODO(samuelmaddock): skip electron worlds
+  return 10;


🤔

This should be calculated to skip the isolated world rather than a hardcoded magic number

We'll want to use World::ISOLATED_WORLD_EXTENSIONS

atom/renderer/renderer_client_base.cc

MarshallOfSound · 2019-04-28T09:23:16Z

To avoid editing the original PR body I'd like to suggest slightly modified user facing API.

interface Extension {
  id: string,
  path: string,
  manifest: chrome.Manifest
}

class Session {
  /** Emitted when extension is installed and ready. */
  on(event: 'chrome-extension-will-activate', listener: (event: Event, extension: Extension) => void): this;
  on(event: 'chrome-extension-activated', listener: (event: Event, extension: Extension) => void): this;
  on(event: 'chrome-extension-deactivated', listener: (event: Event, extension: Extension) => void): this;

  /** This should reject non-absolute paths **/
  loadChromeExtension(path: string): Extension;
  removeChromeExtension(extensionId: string): void;
  enableChromeExtension(extensionId: string): void;
  disableChromeExtension(extensionId: string): void;
}

Summary:

All APIs are now chrome scoped to make it clear these are chrome extensions
Added two new events (activated and deactivated) to help with and UI around enable and disable
- Although the internal hooks are activated and deactivated maybe we should expose events called enabled and disabled to make the API nicer 🤔
Added a will-activate event that is preventable (calling event.preventDefault() will prevent the installation). Can be powered by AtomExtensionLoader::PreAddExtension

samuelmaddock · 2019-04-28T17:49:44Z

atom/browser/atom_browser_context.cc

+
+#if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
+  // TODO: Why?
+  extension_system_ = nullptr;


This originates from the extension shell code. It doesn't seem useful from a functional benefit, but maybe rather signifies to a reader that it should no longer be used within that context.

samuelmaddock · 2019-04-28T18:01:32Z

atom/browser/atom_browser_context.cc

+
+  extension_system_ = static_cast<extensions::AtomExtensionSystem*>(
+      extensions::ExtensionSystem::Get(this));
+  extension_system_->InitForRegularProfile(true /* extensions_enabled */);


I think we might need to implement AtomExtensionSystem::InitForIncognitoProfile to fix tests failing at

[958:0428/030848.424091:FATAL:storage_frontend.cc(140)] Check failed: !browser_context_->IsOffTheRecord().

sentialx · 2019-05-12T18:31:27Z

@samuelmaddock Will we be able to handle UI related things like browserAction.onClicked or chrome.tabs.query?

samuelmaddock · 2019-05-12T18:57:07Z

@samuelmaddock Will we be able to handle UI related things like browserAction.onClicked or chrome.tabs.query?

Not with the currently introduced changes. Some chrome.* APIs will be missing until they're implemented specifically for Electron.

I've thought through how chrome.browserAction could work. Providing a custom button element, similar to <webview>, implemented as <browser-action extensionid="name" tabid="1"> would provide a simple API for Electron developers. Bindings similar to Muon's implementation would forward IPC messages to the renderer for connecting the custom element.

I should mention that I've since stopped actively working on this PR as the project I needed it for is no longer using Electron. Without some kind of sponsorship, I don't see this changing. Electron maintainers have shown interest in merging the changes though.

sentialx · 2019-05-19T11:39:12Z

I've created a library for implementing some of Chrome APIs, maybe this could help: https://github.com/wexond/electron-extensions

nornagon · 2019-07-17T21:32:06Z

Okay, this is getting close to ready to land I think. There's still plenty of work left to do but we're approaching a good "step 1".

TODO before landing:

Currently the tests are crashing when in-memory sessions are used (e.g. in the netLog tests, which create an in-memory session). We should fix this so that all the tests pass when the enable_electron_extensions build flag is enabled.
We should have at least a couple more tests. Currently we just test that a content script gets injected. We should also test a few chrome.* APIs that we expect to work.
We're not ready for prime-time yet so we should disable the enable_electron_extensions flag before landing.

(3) will mean that it's going to be easy for the extensions code to break unless we have a CI environment building with extensions enabled, so we'll have to stay on top of keeping it working by running tests on our local machines until we can enable the flag by default (and eventually remove the flag altogether).

…o = true

nornagon · 2019-07-23T00:03:38Z

(1) above is fixed, and there are now a few simple tests. The mac build failure is the autoUpdater spec which is broken on PRs from forks.

BUILD.gn

nornagon · 2019-07-23T00:20:43Z

buildflags/buildflags.gni

+
+  # Enable Chrome extensions support
+  # TODO: Disable before initial merge
+  enable_electron_extensions = true


Suggested change

enable_electron_extensions = true

enable_electron_extensions = false

shell/browser/api/atom_api_web_contents.cc

shell/browser/atom_browser_main_parts.cc

samuelmaddock

Thanks @nornagon for fixing this up. 🎉

samuelmaddock · 2019-07-23T21:39:54Z

spec-main/extensions-spec.ts

-    (session.defaultSession as any).loadChromeExtension(path.join(fixtures, 'extensions', 'simple'))
-    const w = new BrowserWindow({show: false})
+    const customSession = session.fromPartition(`persist:${require('uuid').v4()}`);
+    (customSession as any).loadChromeExtension(path.join(fixtures, 'extensions', 'red-bg'))


Should there be a TODO dropped in here to add type definitions for loadChromeExtension when the extensions build flag is toggled on?

I expect the API surface will change substantially between now and then so I'm inclined to leave it for now.

samuelmaddock · 2019-07-23T21:45:52Z

shell/browser/extensions/api/runtime/atom_runtime_api_delegate.cc

@@ -0,0 +1,55 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.


Is it okay to leave these copyright headers introduced from the extensions shell code? Or should they be replaced with the GitHub copyright?

We have many examples in our code where we have left in the Chromium copyright and/or added additional copyright depending on the person who created the code (eg there is a Slack copyright in some files). All that to say, its fine to leave as is unless an author wants to add their copyright.

spec-main/fixtures/extensions/chrome-storage/main.js

jkleinsc

Overall LGTM. The only nit I would have is whether or not we should continue using the "Atom" prefix for new classes (vs "Electron" prefix).

jkleinsc · 2019-07-24T14:55:57Z

shell/browser/extensions/api/runtime/atom_runtime_api_delegate.cc

@@ -0,0 +1,55 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.


We have many examples in our code where we have left in the Chromium copyright and/or added additional copyright depending on the person who created the code (eg there is a Slack copyright in some files). All that to say, its fine to leave as is unless an author wants to add their copyright.

jkleinsc · 2019-07-24T14:59:40Z

shell/browser/extensions/api/runtime/atom_runtime_api_delegate.cc

+
+namespace extensions {
+
+AtomRuntimeAPIDelegate::AtomRuntimeAPIDelegate(


If we are adding new classes, should we still use the "Atom" prefix or move over to "Electron" prefix?

I thought we were going to stop prefixing, that's what namespacing is for 😆

cc @deepak1556 I think we talked about this

gonna leave this for now and it can get caught when we do the first sweep.

spec-main/api-net-log-spec.js

release-clerk · 2019-07-24T23:01:12Z

No Release Notes

xylobol · 2019-10-09T15:31:15Z

Anything new on this? I'd love to see this merged. I'd love to contribute but I'm not very well versed in C++ and C.

davwheat · 2019-10-10T21:57:53Z

@xylobol It is merged...

xylobol · 2019-10-10T22:31:41Z

I can't believe I missed that. I need to stop looking through PRs before I've had my morning drink.

electron-cation bot added new-pr 🌱 PR opened in the last 24 hours and removed new-pr 🌱 PR opened in the last 24 hours labels Mar 18, 2019

This was referenced Mar 25, 2019

Extension support minbrowser/min#1

Open

Improve browser security samuelmaddock/metastream#1

Closed

Extensions? beakerbrowser/beaker#230

Open

Add support for Chrome APIs wexond/browser-base#110

Open

samuelmaddock changed the title ~~WIP: Add extensions dependency~~ WIP: feat: Add chromium extensions dependency bindings Mar 26, 2019

samuelmaddock marked this pull request as ready for review March 26, 2019 23:37

ckerr requested a review from a team March 27, 2019 19:08

samuelmaddock force-pushed the extensions-dep branch 3 times, most recently from c1d32ae to c46b7e0 Compare March 29, 2019 22:13

samuelmaddock changed the title ~~WIP: feat: Add chromium extensions dependency bindings~~ feat: Add chromium extensions dependency bindings [WIP] Mar 30, 2019

MarshallOfSound mentioned this pull request Mar 30, 2019

fix: make devtools extensions load correctly #17614

Merged

codebytere changed the title ~~feat: Add chromium extensions dependency bindings [WIP]~~ [wip] feat: Add chromium extensions dependency bindings Apr 1, 2019

codebytere added the wip ⚒ label Apr 1, 2019

samuelmaddock mentioned this pull request Apr 6, 2019

feat: add support for content scripts 'all_frames' option #17258

Merged

5 tasks

ckerr reviewed Apr 24, 2019

View reviewed changes

MarshallOfSound force-pushed the extensions-dep branch 2 times, most recently from 030a062 to 8176fbd Compare April 28, 2019 09:17

samuelmaddock commented Apr 28, 2019

View reviewed changes

jkleinsc mentioned this pull request May 23, 2019

Enable PDF Viewer #12337

Closed

nornagon mentioned this pull request May 30, 2019

refactor: simplify content script injection #18532

Merged

6 tasks

samuelmaddock requested a review from a team as a code owner June 3, 2019 18:58

actually test a thing

04905e4

nornagon added 2 commits July 17, 2019 15:49

fetch original context for services with kServiceRedirectedInIncognit…

df4d598

…o = true

test that extensions get loaded in the correct session

dd186b1

nornagon changed the title ~~[wip] feat: Add chromium extensions dependency bindings~~ feat: Add chromium extensions dependency bindings Jul 18, 2019

nornagon removed the wip ⚒ label Jul 18, 2019

nornagon changed the title ~~feat: Add chromium extensions dependency bindings~~ feat: preliminary support for //extensions Jul 18, 2019

mark browsercontext as live earlier in initialization

c0e96a3

nornagon added 4 commits July 22, 2019 17:14

add test for chrome.runtime.getManifest()

5b841f4

test a couple more simple chrome.runtime APIs

1b5446c

make chrome.storage work

072d48c

add todo for runtime.getPlatformInfo

bebf359

nornagon reviewed Jul 23, 2019

View reviewed changes

nornagon added 2 commits July 22, 2019 17:33

address my own review comments

a38a935

remove bad merge bits

984d8bb

samuelmaddock commented Jul 23, 2019

View reviewed changes

remove extra console.logs

5114aab

jkleinsc reviewed Jul 24, 2019

View reviewed changes

disable extensions by default

8478256

nornagon approved these changes Jul 24, 2019

View reviewed changes

make deps unconditional

823dcc3

nornagon merged commit 9597729 into electron:master Jul 24, 2019

nornagon mentioned this pull request Jul 25, 2019

Tracking issue: //extensions integration #19447

Closed

11 tasks

pmario mentioned this pull request Jul 26, 2019

does beaker has chromium ? beakerbrowser/beaker#1442

Closed

samuelmaddock mentioned this pull request Oct 19, 2020

Tracking Electron contributions samuelmaddock/electron-browser-shell#5

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: preliminary support for //extensions #17440

feat: preliminary support for //extensions #17440

samuelmaddock commented Mar 18, 2019 •

edited

ckerr commented Mar 27, 2019

ckerr left a comment

ckerr Apr 24, 2019

ckerr Apr 24, 2019

samuelmaddock Apr 24, 2019

ckerr Apr 24, 2019

MarshallOfSound Apr 28, 2019

nornagon Jul 15, 2019

ckerr Apr 24, 2019

samuelmaddock Apr 24, 2019

MarshallOfSound commented Apr 28, 2019 •

edited

samuelmaddock Apr 28, 2019

samuelmaddock Apr 28, 2019

sentialx commented May 12, 2019

samuelmaddock commented May 12, 2019

sentialx commented May 19, 2019

nornagon commented Jul 17, 2019 •

edited

nornagon commented Jul 23, 2019

nornagon Jul 23, 2019

samuelmaddock left a comment

samuelmaddock Jul 23, 2019

nornagon Jul 23, 2019

samuelmaddock Jul 23, 2019

nornagon Jul 23, 2019

jkleinsc Jul 24, 2019

jkleinsc left a comment

jkleinsc Jul 24, 2019

jkleinsc Jul 24, 2019

MarshallOfSound Jul 24, 2019

nornagon Jul 24, 2019

release-clerk bot commented Jul 24, 2019

xylobol commented Oct 9, 2019

davwheat commented Oct 10, 2019

xylobol commented Oct 10, 2019

	enable_electron_extensions = true
	enable_electron_extensions = false


		namespace extensions {

		AtomRuntimeAPIDelegate::AtomRuntimeAPIDelegate(

feat: preliminary support for //extensions #17440

feat: preliminary support for //extensions #17440

Conversation

samuelmaddock commented Mar 18, 2019 • edited

Description of Change

Overview

Summary of changes

What's included

What's not included (yet)

What won't be included

Usage

Architecture

Browser

Common

Renderer

Bindings API design (WIP)

Roadmap to merging

Followup roadmap

Checklist

Release Notes

ckerr commented Mar 27, 2019

ckerr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarshallOfSound commented Apr 28, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sentialx commented May 12, 2019

samuelmaddock commented May 12, 2019

sentialx commented May 19, 2019

nornagon commented Jul 17, 2019 • edited

nornagon commented Jul 23, 2019

Choose a reason for hiding this comment

samuelmaddock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkleinsc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

release-clerk bot commented Jul 24, 2019

xylobol commented Oct 9, 2019

davwheat commented Oct 10, 2019

xylobol commented Oct 10, 2019

samuelmaddock commented Mar 18, 2019 •

edited

MarshallOfSound commented Apr 28, 2019 •

edited

nornagon commented Jul 17, 2019 •

edited